Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(hardware): add csv file logging capability to capacitive_probe method #14785

Merged
merged 23 commits into from
Jun 5, 2024
Merged

feat(hardware): add csv file logging capability to capacitive_probe method #14785

merged 23 commits into from
Jun 5, 2024

Conversation

pmoegenburg
Copy link
Contributor

@pmoegenburg pmoegenburg commented Apr 3, 2024 •
edited
Loading

Overview

This PR, along with Opentrons/ot3-firmware#768, adds csv file logging capability to the capacitive_probe ot3api method. This work utilizes the work that adds the same capability to the liquid_probe process. The MoveGroupSingleAxisStep, AddSensorLinearMoveRequest, and SendAccumulatedSensorDataRequest classes were modified to include a sensor_type data field. The capacitive_probe_ot3_tunable script was added for testing.

Test Plan

  • Ran script without csv logging and detected saltwater liquid height
  • Ran script with csv loffing and detected saltwater liquid height

Changelog

Review requests

Risk assessment

@pmoegenburg pmoegenburg added feature Ticket is a feature request / PR introduces a feature firmware hardware embedded labels Apr 3, 2024
@pmoegenburg pmoegenburg self-assigned this Apr 3, 2024
@pmoegenburg pmoegenburg requested a review from a team as a code owner April 3, 2024 03:20
@codecov Codecov
Copy link

codecov bot commented Apr 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 60.15%. Comparing base (ccc944e) to head (22e65ec).
Report is 122 commits behind head on edge.

❗ Current head 22e65ec differs from pull request most recent head ca7ef59

Please upload reports for the commit ca7ef59 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #14785       +/-   ##
===========================================
- Coverage   92.43%   60.15%   -32.29%     
===========================================
  Files          77      189      +112     
  Lines        1283    10587     +9304     
===========================================
+ Hits         1186     6369     +5183     
- Misses         97     4218     +4121
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)
hardware 55.70% <83.33%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
.../opentrons_hardware/firmware_bindings/constants.py 99.29% <100.00%> (ø)
.../firmware_bindings/messages/message_definitions.py 97.62% <100.00%> (ø)
...ns_hardware/firmware_bindings/messages/messages.py 91.66% <ø> (ø)
...ns_hardware/firmware_bindings/messages/payloads.py 96.04% <100.00%> (ø)
...ware/opentrons_hardware/hardware_control/motion.py 83.14% <100.00%> (ø)
...ons_hardware/hardware_control/move_group_runner.py 92.12% <100.00%> (ø)
...rdware/opentrons_hardware/sensors/sensor_driver.py 82.69% <ø> (ø)
...pentrons_hardware/hardware_control/tool_sensors.py 85.71% <80.76%> (ø)

... and 104 files with indirect coverage changes

@pmoegenburg pmoegenburg requested a review from a team as a code owner April 9, 2024 19:31
sfoster1
sfoster1 requested changes Apr 12, 2024
View reviewed changes
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good but definitely some stuff to change.

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
api/src/opentrons/config/defaults_ot3.py Outdated Show resolved Hide resolved
api/src/opentrons/config/defaults_ot3.py Outdated Show resolved Hide resolved
api/src/opentrons/config/types.py Outdated Show resolved Hide resolved
hardware/opentrons_hardware/hardware_control/tool_sensors.py Outdated Show resolved Hide resolved
hardware/opentrons_hardware/hardware_control/tool_sensors.py Outdated Show resolved Hide resolved
hardware/opentrons_hardware/hardware_control/tool_sensors.py Outdated Show resolved Hide resolved
hardware/opentrons_hardware/hardware_control/tool_sensors.py Outdated Show resolved Hide resolved
hardware/opentrons_hardware/hardware_control/tool_sensors.py Outdated Show resolved Hide resolved
@pmoegenburg pmoegenburg requested a review from sfoster1 May 7, 2024 17:44
@pmoegenburg pmoegenburg mentioned this pull request Jun 3, 2024
Merged
2 tasks
sfoster1
sfoster1 approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, nice!

ryanthecoder
ryanthecoder approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@ryanthecoder ryanthecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pmoegenburg pmoegenburg merged commit b2c6390 into edge Jun 5, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DerekMaggio DerekMaggio DerekMaggio left review comments

@ryanthecoder ryanthecoder ryanthecoder approved these changes

@sfoster1 sfoster1 sfoster1 approved these changes

@caila-marashaj caila-marashaj Awaiting requested review from caila-marashaj

Assignees

@pmoegenburg pmoegenburg

Labels
embedded feature Ticket is a feature request / PR introduces a feature firmware hardware
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmoegenburg @sfoster1 @ryanthecoder @DerekMaggio